-
Notifications
You must be signed in to change notification settings - Fork 861
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix missing increment of the reference count on every call to startup() #3100
base: master
Are you sure you want to change the base?
Conversation
As for me it looks ok, but Max will be back in the second half of January. |
Still has bugs with existing code, as per the issue discussion |
also change how the implicit startup is tracked. The extra change is needed since the startup() calls in srt::CUDT::socket() and srt::CUDT::createGroup would endlessly increment the startup counter as sockets were created. This also adds handling for the case where the user code fails to call srt_startup when initially using the library, but does later on. It better matches up the implicit startup() with an implicit cleanup(). fixes Haivision#3098
I think this modified solution is reasonable to handle the remaining issues. |
return 1; | ||
return; | ||
m_bImplicitStartup = true; | ||
startup(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Double locking on m_InitLock
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh is the lock primitive used in this library not recursive?
Wait, there's one thing I don't understand. Why do you need to prevent the call to cleanup()? The call to cleanup() should be able to be done multiple times, even if at least one of the series has done the real cleanup. The only rule was that there should be done as many (meaning, the minimum) cleanup calls as there was startup calls, but extra cleanup() calls, which actually do nothing, should be allowed. Have you found a problem with extra cleanups? |
This doesn't disallow extra cleanup in general, that allowance remains in place. The change here though is that it avoids an unpaired cleanup from internal code. Doing a cleanup internally when a startup wasn't done internally can hide bugs in the API users code. And since the cleanup done in |
also change how the implicit startup is tracked.
The extra change is needed since the startup() calls in
srt::CUDT::socket()
and
srt::CUDT::createGroup
would endlessly increment the startup counter as sockets were created.
This also adds handling for the case where the user code fails to call
srt_startup when initially using the library, but does later on.
It better matches up the implicit startup() with an implicit cleanup().
fixes #3098